NO-JIRA: Add API server stabilization wait to serviceaccountissuer tests#1997
Conversation
|
@gangwgr: This pull request explicitly references no jira issue. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughRefactored E2E tests: Ginkgo test in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)test/e2e/serviceaccountissuer.go (1)
🔇 Additional comments (4)
Comment |
openshift-ci
Bot
added
the
approved
gangwgr
changed the title
gangwgr
force-pushed
the
serviceaccountissuer-test
branch
4 times, most recently
from
1ff36c1 to
2eb5092
Compare
gangwgr
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@gangwgr When the e2e-gcp-operator-serial-ote runs passed, yon can unhold. |
gangwgr
force-pushed
the
serviceaccountissuer-test
branch
from
2eb5092 to
7696fbd
Compare
gangwgr
force-pushed
the
serviceaccountissuer-test
branch
from
7696fbd to
ac1bfe7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/serviceaccountissuer.go (1)
42-55: Missing stabilization waits after first and second issuer changes.Per the PR rationale, tests failed because validations ran before the API server stabilized. However,
WaitForAPIServerToStabilizeOnTheSameRevisionis only called after the final phase. The first two test functions (testServiceAccountIssuerFirstIssuerandtestServiceAccountIssuerSecondIssuer) may still encounter the same race condition sincepollForOperandIssueronly checks the configmap contents, not full API server rollout completion.Consider adding stabilization waits after each
setServiceAccountIssuercall:func testServiceAccountIssuerFirstIssuer(t testing.TB) { kubeConfig, err := testlibrary.NewClientConfigForTest() require.NoError(t, err) kubeClient, err := clientcorev1.NewForConfig(kubeConfig) require.NoError(t, err) authConfigClient, err := configv1.NewForConfig(kubeConfig) require.NoError(t, err) setServiceAccountIssuer(t, authConfigClient, "https://first.foo.bar") err = pollForOperandIssuer(t, kubeClient, []string{"https://first.foo.bar", "https://kubernetes.default.svc"}) require.NoError(t, err, "pollForOperandIssuer failed") + testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) }Also applies to: 57-70
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
test/e2e/serviceaccountissuer.gotest/e2e/serviceaccountissuer_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/serviceaccountissuer.gotest/e2e/serviceaccountissuer_test.go
🧬 Code graph analysis (1)
test/e2e/serviceaccountissuer.go (1)
pkg/operator/operatorclient/interfaces.go (1)
TargetNamespace(7-7)
🔇 Additional comments (3)
test/e2e/serviceaccountissuer.go (2)
30-39: Clean consolidation of lifecycle tests.The use of
g.By()steps within a singleItblock clearly delineates each phase while ensuring sequential execution. The 60m timeout appropriately accommodates the multiple stabilization periods.
85-86: Good addition of stabilization wait.Adding
WaitForAPIServerToStabilizeOnTheSameRevisionafter the final configuration change ensures the API server has fully rolled out before test completion.test/e2e/serviceaccountissuer_test.go (1)
7-28: Well-structured test consolidation with clear documentation.The comment clearly explains the temporary migration state and the three-phase lifecycle being tested. Using
t.Runsubtests maintains sequential execution while providing clear test output for each phase.Note: The stabilization wait concern raised in
serviceaccountissuer.goapplies here as well since these tests call the same helper functions.
gangwgr
force-pushed
the
serviceaccountissuer-test
branch
from
ac1bfe7 to
aca76e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/serviceaccountissuer.go (1)
42-55: Missing stabilization waits in the first two test phases.The PR objective states stabilization waits should be added "after each setServiceAccountIssuer operation," but
testServiceAccountIssuerFirstIssuerandtestServiceAccountIssuerSecondIssuerdon't callWaitForAPIServerToStabilizeOnTheSameRevision. This inconsistency could still cause intermittent failures when the next phase validates before the API server stabilizes.🔧 Proposed fix to add stabilization waits to all phases
In
testServiceAccountIssuerFirstIssuer:func testServiceAccountIssuerFirstIssuer(t testing.TB) { kubeConfig, err := testlibrary.NewClientConfigForTest() require.NoError(t, err) kubeClient, err := clientcorev1.NewForConfig(kubeConfig) require.NoError(t, err) authConfigClient, err := configv1.NewForConfig(kubeConfig) require.NoError(t, err) setServiceAccountIssuer(t, authConfigClient, "https://first.foo.bar") err = pollForOperandIssuer(t, kubeClient, []string{"https://first.foo.bar", "https://kubernetes.default.svc"}) require.NoError(t, err, "pollForOperandIssuer failed") + // Wait for API server to stabilize after configuration change + testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) }In
testServiceAccountIssuerSecondIssuer:func testServiceAccountIssuerSecondIssuer(t testing.TB) { kubeConfig, err := testlibrary.NewClientConfigForTest() require.NoError(t, err) kubeClient, err := clientcorev1.NewForConfig(kubeConfig) require.NoError(t, err) authConfigClient, err := configv1.NewForConfig(kubeConfig) require.NoError(t, err) setServiceAccountIssuer(t, authConfigClient, "https://second.foo.bar") err = pollForOperandIssuer(t, kubeClient, []string{"https://second.foo.bar", "https://first.foo.bar", "https://kubernetes.default.svc"}) require.NoError(t, err, "pollForOperandIssuer failed") + // Wait for API server to stabilize after configuration change + testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) }Also applies to: 57-70
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
test/e2e/serviceaccountissuer.gotest/e2e/serviceaccountissuer_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/serviceaccountissuer.gotest/e2e/serviceaccountissuer_test.go
🧬 Code graph analysis (1)
test/e2e/serviceaccountissuer.go (1)
pkg/operator/operatorclient/interfaces.go (1)
TargetNamespace(7-7)
🔇 Additional comments (4)
test/e2e/serviceaccountissuer.go (3)
21-21: LGTM!The import for
testlibraryapiis correctly added to support the stabilization wait helper.
30-39: LGTM!Good restructuring. Combining the three test phases into a single
Itblock withBy()steps provides a clear lifecycle narrative and ensures sequential execution. The[Timeout:30m]annotation appropriately accommodates the longer stabilization waits.
85-86: LGTM!Correctly adds the stabilization wait after resetting to the default issuer.
test/e2e/serviceaccountissuer_test.go (1)
7-28: LGTM!Good consolidation. The subtests correctly run sequentially (no
t.Parallel()), maintaining the lifecycle dependency between phases. The comment clearly explains the temporary dual-framework situation and the three-phase lifecycle being tested.
|
/test e2e-aws-ovn |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, wangke19 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn-serial-2of2 |
|
@gangwgr: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| err = pollForOperandIssuer(t, kubeClient, []string{"https://kubernetes.default.svc"}) | ||
| require.NoError(t, err, "pollForOperandIssuer failed") | ||
| // Wait for API server to stabilize after configuration change | ||
| testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) |
There was a problem hiding this comment.
is this really required ? (it wasn't before)
There was a problem hiding this comment.
Yes it is required, kas getting rollout after after config change which intermittently impacting other cases.
4 participants
This PR improves the reliability of the serviceaccountissuer e2e tests by ensuring the API server has stabilized after configuration changes before proceeding with validation.
Changes
WaitForAPIServerToStabilizeOnTheSameRevisioncalls after eachsetServiceAccountIssueroperation to ensure the API server has rolled out the configuration change[Timeout:30m]to accommodate the additional wait timetestlibraryapifromlibrary-go/test/library/apiserverfor the wait helperMotivation
The serviceaccountissuer tests were failing intermittently because they were attempting to verify configuration changes before the API server had fully processed and stabilized on the new configuration. This change ensures we wait for the rollout to complete before validating the expected issuer values.
Fixed Files
Before: Three separate g.It test blocks that could be run individually
After: Single combined test that runs all three phases sequentially:
Before: Three separate Test* functions
After: Single combined test TestServiceAccountIssuer that runs all three phases sequentially:
Why This Fix Was Needed
The tests depend on sequential execution because:
Running them individually would fail because the second test needs the first issuer to already exist.